ICU-22763 MF2: Handle time zones correctly in :datetime #3012

catamorphism · 2024-05-16T20:53:26Z

Previously, the time zone components of date/time literals were ignored. In order to store the time zone properly for re-formatting, this change also changes the representation of Formattable date values to use a GregorianCalendar rather than a UDate.

This is a public API change and a design doc can be found in the "ICU 76 API proposal status" document (and was also emailed to the icu-design list on April 29.)

In the TC discussion on May 9, there was some uncertainty about whether to use GregorianCalendar or Calendar as the return type of message2::Formattable::getDate(). I'm leaving it as GregorianCalendar for now but can change it if necessary.

Checklist

Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22763
Required: The PR title must be prefixed with a JIRA Issue number.
Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
Required: Each commit message must be prefixed with a JIRA Issue number.
Issue accepted (done by Technical Committee after discussion)
Tests included, if applicable
API docs and/or User Guide docs changed or added, if applicable

jira-pull-request-webhook · 2024-05-16T21:04:10Z

Notice: the branch changed across the force-push!

icu4c/source/i18n/messageformat2_function_registry.cpp is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

Previously, the time zone components of date/time literals were ignored. In order to store the time zone properly for re-formatting, this change also changes the representation of `Formattable` date values to use a `GregorianCalendar` rather than a `UDate`. (This is a public API change.)

jira-pull-request-webhook · 2024-05-16T21:27:46Z

Notice: the branch changed across the force-push!

icu4c/source/i18n/unicode/messageformat2_formattable.h is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

catamorphism · 2024-05-16T22:03:51Z

cc @echeran @mihnita

markusicu · 2024-05-23T16:14:43Z

@FrankYFTang setting you as the main reviewer because Mihai is ooo for a while soon. Note that the API change proposal is still under discussion.

FrankYFTang · 2024-06-13T20:59:51Z

Here is my view

Currently, the Message Format proposal only accept string in ISO8601, without the support of timeZone Extension and Calendar extension as stated in RFC 9557 https://datatracker.ietf.org/doc/html/rfc9557
therefore, the only additional information it need to and should be allowed to get back is for the case of the string has UTC offset, and ISO8601 only allow it to be in minutes precision.
"
5.3.4.1 Time shift between local time scale and UTC
The time shift between the local time scale and UTC can be expressed in hours and minutes, or hours only
"
therefore, the requirement is to allow the caller to get back this information only, but not open up a can of worm that more than that. and we should also allow the interface to distinguish the case that the string has Z (UTC) or no Z.
Currently, in ICU, we can get the zone offset from Calendar.get(UCAL_ZONE_OFFSET) and the DST offset from Calendar.get(UCAL_DST_OFFSET) and the UTC offset is the sum of that in millseconds
So I think we should define a new type for input and return, which has
a UDate
int32_t rawOffsetGMT - represent the zone offset + dst offset in millisecond
and this timezone (2 above) can be used ot create a TimeZone by

icu::SimpleTimeZone::SimpleTimeZone ( int32_t rawOffsetGMT,
const UnicodeString & ID
)

FrankYFTang · 2024-06-13T21:01:35Z

Maybe something like

struct {
  UDate date;
  int32_t rawOffsetGMT;
} DateAndGMTOffset

?

aphillips · 2024-06-13T21:49:41Z

Note that MF2 will not stay with only supporting vanilla 8601 strings. Real time zone support is important. An offset is insufficient.

FrankYFTang · 2024-06-13T22:33:25Z

Note that MF2 will not stay with only supporting vanilla 8601 strings. Real time zone support is important. An offset is insufficient.

that is what I asked this morning and the answer I got back is only vanilla 8601 strings. if MF2 is going to support RFC 9557 https://datatracker.ietf.org/doc/html/rfc9557 then we will need to consider not just timezone extension in RFC9557 but also calendar extension in RFC 9557. Therefore, it will be a super bad idea to use GregorianCalendar since the calendar could be in a different calendar system.

We should consider the following cases

the string is with a Z
the string is with a UTC offset
(both 1 and 2 are just vanilla 8601 strings)
the string is with timeZone extension as in RFC 9557
the string is with calendar extnesion as in RFC 9557
any combination of 1,2,3,4

For 4, we need a string to indicate the real time zone name but not more than that
For 5, we need a string to indicate the calendar name but not more than tat

how about

struct {
  UDate date;
  int32_t rawOffsetGMT;
  const char* zoneName; // "Z" if UTC, nullptr if timeZone offset, other if from RFC9557
  const char* calendarName; // nullptr if not specified
} DateInfo

catamorphism · 2024-06-13T23:45:17Z

that is what I asked this morning and the answer I got back is only vanilla 8601 strings.

I apologize for giving the wrong answer -- I was answering based on the existing spec. @aphillips is more of an authority than me about how the spec will evolve in the future.

aphillips · 2024-06-14T15:58:05Z

(personal response, chair hat off)

The offset is irrelevant. The only thing an offset is useful for is adjusting the date value (the incremental time value--Temporal calls this an Instant). Once the incremental time value is computed, you can safely forget the offset. (In most situations, I would argue that the value should be normalized to UTC and the implementation should forget the offset) The time zone, however, is used in expression of the date value later. The time zone can also be changed or removed ("floating" the value).

The use of non-Gregorian calendars in date/time serializations is more problematic: there's not a lot of implementation experience, particularly with non-binary multi-era calendars. It's usually better (or at least more common) to use a proleptic Gregorian calendar (i.e. 8601) on the wire (especially for incremental time values!) and convert using calendar rules for processing/display. Perhaps this will evolve and mature further? My experience here is limited.

There also needs to be a concept of a floating time value (one not tied to specific instants on the timeline). This could use null for the time zone. Perhaps:

struct {
  UDate date; // what is a `UDate` anyway? Is it millis since the Unix epoch?
  const char* zoneName; // IANA tz name; "UTC" if UTC; nullptr if value is floating
  const char* calendarName; // nullptr if not specified; proleptic Gregorian (8601) calendar is default
} DateInfo

In any case, this was a significant gap in the MF2 tech preview. It's important for us to add time zone management to MF2 before LDML46 and the ICU4C implementation should be designed/adapted with that in mind.

catamorphism · 2024-06-17T18:23:43Z

Thanks for the feedback, @aphillips and @FrankYFTang . I'll take a look at these options and update the design doc accordingly.

My first thought, though, is why not something like:

struct {
  UDate date;
  SimpleTimeZone tz; // Captures offset and time zone name
  const char* calendarName; // nullptr if not specified
} DateInfo

Why not re-use the existing SimpleTimeZone class?

catamorphism · 2024-06-18T21:46:10Z

@FrankYFTang I've made changes to match the suggestions; can you take another look? I've also updated the design proposal in the "ICU 76 API proposal status" doc.

FrankYFTang · 2024-06-20T23:13:30Z

icu4c/source/i18n/unicode/messageformat2_formattable.h

+         * @internal ICU 76 technology preview
+         * @deprecated This API is for technology preview only.
+         */
+        UnicodeString calendarName;


I do not understand why you need createTimeZone() here since all the information you need can be derived from zoneName itself. I will prefer we not adding more function if it is really needed.

I included it because to call DateFormat::format() to eventually format the date, a TimeZone object has to be constructed (to pass to DateFormat's adoptTimeZone() or setTimeZone()) method. But it doesn't have to be a method of the DateInfo struct (changed in 1c68d51).

FrankYFTang · 2024-06-20T23:19:26Z

icu4c/source/i18n/unicode/messageformat2_formattable.h

+         * @internal ICU 76 technology preview
+         * @deprecated This API is for technology preview only.
+         */
+        UnicodeString zoneName;


should this be const?

DateInfo is movable (because it can appear inline in Formattable, which is movable), so the members can't be const. I could change DateInfo to a class and make the members private, if you think that would be better.

srl295

Generally looking GTM, some comments

icu4c/source/test/testdata/message2/more-functions.json

srl295 · 2024-06-28T21:04:48Z

icu4c/source/i18n/unicode/messageformat2_formattable.h

+         * @internal ICU 76 technology preview
+         * @deprecated This API is for technology preview only.
+         */
+        UnicodeString calendarName;


What is the calendarName supposed to map to? Is it the 'old style name' (gregorian) matching

icu/icu4c/source/i18n/unicode/calendar.h

Line 1297 in cf7ff1b

virtual const char * getType() const = 0;

or a bcp47 -u-ca type (gregory)?

If it's from the locale, wouldn't that be visible from the locale of the message format itself?

The spec currently says that it's this, but it's not fully defined in v45 (the calendar option is reserved for future standardization). Probably we want to support whatever JS Temporal does or at least be compatible with that.

It should probably link there, as a 'calendar type' in ICU4C is one of the old types generally.

@srl295 Added a comment in 82f9ddc - let me know if that looks OK.

srl295 · 2024-06-28T21:06:56Z

needs squash

Co-authored-by: Steven R. Loomis <[email protected]>

catamorphism · 2024-07-19T00:43:46Z

@FrankYFTang Are you OK with the current state of the design doc for this PR? The JIRA ticket shows as "Accepted", but I've made changes to the design doc since your last comment.

icu4c/source/i18n/messageformat2_function_registry.cpp

icu4c/source/i18n/messageformat2_formattable.cpp

FrankYFTang · 2024-08-08T22:36:25Z

icu4c/source/i18n/messageformat2_function_registry.cpp

+        return 0;
+    }
+    // 'zzzz' to handle the case where the timezone offset follows the ISO 8601 datetime
+    if (sourceStr.length() > 25) {


Could you explain where the 25 came from? How do you conclude it need to > 25? and 25 is enough?

https://unicode.org/reports/tr35/tr35-dates.html#Contents show

zzzz Pacific Daylight Time The long specific non-location format. Where that is unavailable, falls back to the long localized GMT format ("OOOO").

and

OOOO GMT-08:00 The long localized GMT format.

I suppose it doesn't really need to check the length at all, just whether the last character is 'Z'. Changed that in c63c6b5.

…zoneId

catamorphism · 2024-08-08T22:47:05Z

In 197caef, I added some comments explaining the meaning of the fields in the DateInfo struct. These can be checked against the code in messageformat2_formattable.cpp that formats an existing DateInfo struct, and that parses a date literal into a DateInfo -- the two locations where the DateFormat::adoptTimeZone() method is called.

catamorphism · 2024-08-08T22:49:18Z

I also removed the calendarName field in 9800253. On further thought, it doesn't seem necessary; a DateInfo represents a parsed date literal, and date literal strings don't include a calendar name. In the future, calendar will be an option on the :datetime formatter. Then it will be handled the same way as all other options, and the return value from the formatter will represent the previously passed-in calendar by default. Therefore, I don't think we need it in the DateInfo struct, which is meant to represent the parsed version of a date literal string.

catamorphism · 2024-08-08T22:59:04Z

This needs to be rebased because of #3050 moving where the test data is, but I'll do that at the very end in order to avoid losing review comments.

FrankYFTang · 2024-08-12T18:26:51Z

icu4c/source/i18n/messageformat2_function_registry.cpp

+    if (U_FAILURE(errorCode)) {
+        return 0;
+    }
+    return dateParser->parse(sourceStr, errorCode);


dateParser->parse() is a const method, which mean it could be shared across thread, right?

there are only total 4 calls into tryPattern() in this method

tryPattern("YYYY-MM-dd'T'HH:mm:ss"
tryPattern("YYYY-MM-dd"
tryPattern("YYYY-MM-dd'T'HH:mm:ssZ"
tryPattern("YYYY-MM-dd'T'HH:mm:sszzzz"

So there are total 4 possible pat , right?
Currently, the creation of the DateFormat is inside the tryPattern()
Everytime the tryPattern() got called, the code need to create a SimpleDateFormat and later destroy it
but there are only total 4 possible SimpleDateFormat needed to be created and the usage are thread safe (see my point 1 above)
So... should we have an lazy initialization routine which create these four SimpleDateFormat and cache it globally, and reuse them later. The creation need to be protected to be init once and therefore mutex to prevent different threads create them concurrently, but then we can reuse them across thread and destroy upon lib clean up.

In this way, we do not need to repeatly create and destroy the SimpleDateFormat

Yes, that seems reasonable. I'll try it.

OK, done in 8d4da42. I'm not sure of how to unit-test this (i.e. testing that the DateFormats really are initialized only once), but I'll look for examples in the test suite.

FrankYFTang · 2024-08-12T18:28:10Z

icu4c/source/i18n/messageformat2_function_registry.cpp

+    if (len <= 6) {
+        return false;
+    }
+    return ((sourceStr[len - 6] == PLUS || sourceStr[len - 6] == HYPHEN)


Please add a comment in the beginning of this function to explain what is the expected input value and the output

Done in 4a7da29

…tialize them

catamorphism · 2024-08-13T23:37:27Z

By the way, I'll be on vacation from now until September 9, so I won't see further comments until I return.

FrankYFTang · 2024-08-14T01:19:36Z

icu4c/source/i18n/messageformat2_function_registry_internal.h

@@ -57,6 +57,15 @@ namespace message2 {

            DateTimeType type;
            DateTimeFactory(DateTimeType t) : type(t) {}
+


so.. you store 4 pointer per DateTimeFactory object. But in messageformat2_function_registry.cpp you create multiple DateTimeFactory object, each object will crate the same four DateFormat. But there are no differences between them from the multiple DateTimeFactory . Could you use InitOnce and cleanup code tostore that four DateFormat to be globally shared ?

@catamorphism search for icu::UInitOnce and ucln_common_registerCleanup for examples.

Done in da9307c

After implementing that change and running under asan, I noticed a use-after-free error. This required me to change some code in tznames.cpp that dealt with reference counting on TimeZoneNamesDelegate objects. The deleteTimeZoneNamesCacheEntry() function in that file was assuming there are no other references to the cache entry. But the new global DateFormat object holds a reference to a cache entry, and the new cleanup function I added was getting called after the timeZoneNames_cleanup() function and hence after the cache was already deleted. So I had to change both deleteTimeZoneNamesCacheEntry() and ~TimeZoneNamesDelegate() to check the ref counts. I made this a separate patch (for ease of review), 1b8e43f.

catamorphism · 2024-10-07T18:43:50Z

@FrankYFTang ping?

catamorphism force-pushed the icu-22763 branch from bdca226 to ca6d29d Compare May 16, 2024 21:04

catamorphism force-pushed the icu-22763 branch from ca6d29d to 4bc0354 Compare May 16, 2024 21:27

catamorphism marked this pull request as ready for review May 16, 2024 22:03

markusicu requested review from echeran and mihnita May 23, 2024 16:13

markusicu assigned FrankYFTang May 23, 2024

catamorphism added 9 commits June 18, 2024 11:31

Tests pass with new DateInfo struct

fa6278e

Eliminate GregorianCalendar usage

8451a30

Docs

b4cc119

Remove obsolete comment

eeb534b

Fix comment

8a1638b

Add comments

60a66d9

Avoid creating time zone where not necessary

a65362f

Fix doc comment

3d78033

Fix MSVC compilation errors

804ab9e

FrankYFTang reviewed Jun 20, 2024

View reviewed changes

srl295 reviewed Jun 28, 2024

View reviewed changes

catamorphism and others added 2 commits July 17, 2024 13:35

Update icu4c/source/test/testdata/message2/more-functions.json

5d15835

Co-authored-by: Steven R. Loomis <[email protected]>

Add comment about meaning of calendarName

82f9ddc